-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Validator] - Added Timezone Constraint Validator #13270
Conversation
The test class looks like it's just a copy of the constraint validator class. |
@xabbuh already fixed, but well catched 💃 Should be good now :) |
* @Annotation | ||
* @Target({"PROPERTY", "METHOD", "ANNOTATION"}) | ||
* | ||
* @author Mickaël Andrieu <mickael.andrieu@sensiolabs.com> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was from SL when I did this pull request.
Extracted from a project with project manager agreement.
edit: I've updated, I'm not attached to my "old" email ;)
@hhamon @xabbuh thank for this final review. ping @webmozart : now this is realy done i guess 🐱 |
*/ | ||
class Timezone extends Constraint | ||
{ | ||
public $message = 'The value "{{ value }}" is not a valid timezone'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message should be also added to the translations file (at least the English one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, how about changing the message to "{{ value }}" is not a valid timezone
?
The value "Foo/Bar" is not a valid timezone
doesn't sound right to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jakzal, you're right, I've updated.
Please also add test cases against the other validation APIs |
@stof need help here as I don't know what I need to do, did you have an example ? |
@@ -306,6 +306,10 @@ | |||
<source>The host could not be resolved.</source> | |||
<target>The host could not be resolved.</target> | |||
</trans-unit> | |||
<trans-unit id="80"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The id will need to become 81
.
@mickaelandrieu You can have a look at the |
After 12 months I think ultimately this constraint does not need to be part of the core . If it had been, it would have been merged long time ago (was validated for 2.4, then 2.5, then 2.6, now 2.7 or 3.0 ?). I'm sorry for all who have reviewed this one, but I don't want to "waste" time on this (and I don't want others waste time on this too), because this is not so useful and it's better at this point to just close this. |
comments: was previously, #11010. Re-created because I've deleted my old fork.
ping @webmozart